8369184: SimpleTimeZone equals() Returns True for Unequal Instances with Different hashCode Values #27660
Conversation
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
There was a problem hiding this comment.
Include equals in the test name as well? Perhaps HashCodeEqualsTest or something along those lines.
There was a problem hiding this comment.
Renamed. I left SimpleTimeZone as the test is specific for this class.
| { | ||
| return startMonth ^ startDay ^ startDayOfWeek ^ startTime ^ | ||
| endMonth ^ endDay ^ endDayOfWeek ^ endTime ^ rawOffset; | ||
| int hash = 31 * getID().hashCode() + rawOffset; |
There was a problem hiding this comment.
int hash = Objects.hash(getID(), rawOffset, useDaylight);
return !useDaylight ? hash : 31 * hash + Objects.hash(
startMonth, startDay, startDayOfWeek, startTime, endMonth, endDay, endDayOfWeek, endTime);
Seems reasonable to use Objects.hash here. Could save some lines if wanted?
There was a problem hiding this comment.
Initially, I thought so, but decided to avoid extra array allocation/copy. But on second thought, the instances of this class would seldom be used as hash keys so not that performance critical. Either way is fine with me
Fixing the equals/hashCode contract in the SimpleTimeZone class. The current implementation includes DST rule fields in hash code computation even for zones that do not observe DST, while equals() always considers them. Also correcting the example code in the class description, where it used 20-year-old obsolete "America/Los_Angeles" rule.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27660/head:pull/27660$ git checkout pull/27660Update a local copy of the PR:
$ git checkout pull/27660$ git pull https://git.openjdk.org/jdk.git pull/27660/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27660View PR using the GUI difftool:
$ git pr show -t 27660Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27660.diff
Using Webrev
Link to Webrev Comment